From f1c2bdb783bfcdf7c122e8a73bdce482df02603a Mon Sep 17 00:00:00 2001 From: MEFThunders7035 <95276965+kytpbs@users.noreply.github.com> Date: Tue, 7 May 2024 02:42:42 +0300 Subject: [PATCH 1/9] Copy scheduledCommands before iterating Based on the robot.py commandScheduler implementation. This way we avoid an iterator invalidation bug when a command ends and we delete it. Because we are only copying a stack(most probably) pointer array, it shouldn't have a big impact on performance. --- .../src/main/native/cpp/frc2/command/CommandScheduler.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp index def91d64879..9c9d8d4fb8d 100644 --- a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp +++ b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp @@ -210,8 +210,8 @@ void CommandScheduler::Run() { m_impl->inRunLoop = true; bool isDisabled = frc::RobotState::IsDisabled(); - // Run scheduled commands, remove finished commands. - for (Command* command : m_impl->scheduledCommands) { + // create a new set to avoid iterator invalidation. + for (Command* command : wpi::SmallSet(m_impl->scheduledCommands)) { if (isDisabled && !command->RunsWhenDisabled()) { Cancel(command, std::nullopt); continue; @@ -231,7 +231,6 @@ void CommandScheduler::Run() { } m_impl->endingCommands.erase(command); - m_impl->scheduledCommands.erase(command); for (auto&& requirement : command->GetRequirements()) { m_impl->requirements.erase(requirement); } @@ -241,6 +240,7 @@ void CommandScheduler::Run() { m_impl->ownedCommands.erase(command); } } + m_impl->inRunLoop = false; for (Command* command : m_impl->toSchedule) { From 6607580627e78bf190a968dbca24c1abb29855a3 Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Wed, 22 May 2024 01:27:02 +0300 Subject: [PATCH 2/9] remove concurrent avoiding modification member variables (C++) --- .../cpp/frc2/command/CommandScheduler.cpp | 48 +++---------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp index 9c9d8d4fb8d..0a80572e299 100644 --- a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp +++ b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp @@ -55,14 +55,6 @@ class CommandScheduler::Impl { wpi::SmallVector interruptActions; wpi::SmallVector finishActions; - // Flag and queues for avoiding concurrent modification if commands are - // scheduled/canceled during run - bool inRunLoop = false; - wpi::SmallVector toSchedule; - wpi::SmallVector toCancelCommands; - wpi::SmallVector, 4> toCancelInterruptors; - wpi::SmallSet endingCommands; - // Map of Command* -> CommandPtr for CommandPtrs transferred to the scheduler // via Schedule(CommandPtr&&). These are erased (destroyed) at the very end of // the loop cycle when the command lifecycle is complete. @@ -118,11 +110,6 @@ frc::EventLoop* CommandScheduler::GetDefaultButtonLoop() const { } void CommandScheduler::Schedule(Command* command) { - if (m_impl->inRunLoop) { - m_impl->toSchedule.emplace_back(command); - return; - } - RequireUngrouped(command); if (m_impl->disabled || m_impl->scheduledCommands.contains(command) || @@ -208,10 +195,13 @@ void CommandScheduler::Run() { loopCache->Poll(); m_watchdog.AddEpoch("buttons.Run()"); - m_impl->inRunLoop = true; bool isDisabled = frc::RobotState::IsDisabled(); // create a new set to avoid iterator invalidation. for (Command* command : wpi::SmallSet(m_impl->scheduledCommands)) { + if (!IsScheduled(command)) { + continue; // skip as the normal scheduledCommands was modified + } + if (isDisabled && !command->RunsWhenDisabled()) { Cancel(command, std::nullopt); continue; @@ -224,37 +214,23 @@ void CommandScheduler::Run() { m_watchdog.AddEpoch(command->GetName() + ".Execute()"); if (command->IsFinished()) { - m_impl->endingCommands.insert(command); + m_impl->scheduledCommands.erase(command); command->End(false); for (auto&& action : m_impl->finishActions) { action(*command); } - m_impl->endingCommands.erase(command); for (auto&& requirement : command->GetRequirements()) { m_impl->requirements.erase(requirement); } m_watchdog.AddEpoch(command->GetName() + ".End(false)"); + // remove owned commands after everything else is done m_impl->ownedCommands.erase(command); } } - m_impl->inRunLoop = false; - - for (Command* command : m_impl->toSchedule) { - Schedule(command); - } - - for (size_t i = 0; i < m_impl->toCancelCommands.size(); i++) { - Cancel(m_impl->toCancelCommands[i], m_impl->toCancelInterruptors[i]); - } - - m_impl->toSchedule.clear(); - m_impl->toCancelCommands.clear(); - m_impl->toCancelInterruptors.clear(); - // Add default commands for un-required registered subsystems. for (auto&& subsystem : m_impl->subsystems) { auto s = m_impl->requirements.find(subsystem.getFirst()); @@ -346,24 +322,14 @@ void CommandScheduler::Cancel(Command* command, if (!m_impl) { return; } - if (m_impl->endingCommands.contains(command)) { - return; - } - if (m_impl->inRunLoop) { - m_impl->toCancelCommands.emplace_back(command); - m_impl->toCancelInterruptors.emplace_back(interruptor); - return; - } if (!IsScheduled(command)) { return; } - m_impl->endingCommands.insert(command); + m_impl->scheduledCommands.erase(command); command->End(true); for (auto&& action : m_impl->interruptActions) { action(*command, interruptor); } - m_impl->endingCommands.erase(command); - m_impl->scheduledCommands.erase(command); for (auto&& requirement : m_impl->requirements) { if (requirement.second == command) { m_impl->requirements.erase(requirement.first); From 949a891d65be4b93e8f75d71ec77e54bb65a8114 Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Fri, 11 Oct 2024 05:53:57 +0300 Subject: [PATCH 3/9] add a test to cancel the next command from the first Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com> --- .../cpp/frc2/command/CommandScheduleTest.cpp | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp index 91786770e09..3b5b0a18987 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp @@ -2,6 +2,10 @@ // 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. +#include + +#include + #include #include @@ -102,6 +106,43 @@ TEST_F(CommandScheduleTest, NotScheduledCancel) { EXPECT_NO_FATAL_FAILURE(scheduler.Cancel(&command)); } +TEST_F(CommandScheduleTest, CancelNextCommand) { + CommandScheduler scheduler = GetScheduler(); + + frc2::RunCommand* command1PtrPtr = nullptr; + frc2::RunCommand* command2PtrPtr = nullptr; + auto counterPtr = std::make_shared(0); + + auto command1 = frc2::RunCommand([counterPtr, &command2PtrPtr, &scheduler] { + scheduler.Cancel(command2PtrPtr); + (*counterPtr)++; + }); + auto command2 = frc2::RunCommand([counterPtr, &command1PtrPtr, &scheduler] { + scheduler.Cancel(command1PtrPtr); + (*counterPtr)++; + }); + + command1PtrPtr = &command1; + command2PtrPtr = &command2; + + scheduler.Schedule(&command1); + scheduler.Schedule(&command2); + scheduler.Run(); + + EXPECT_EQ(*counterPtr, 1); + + // only one of the commands should be canceled. + EXPECT_FALSE(scheduler.IsScheduled(&command1) && + scheduler.IsScheduled(&command2)); + // one of the commands shouldn't be canceled because the other one is canceled + // first + EXPECT_TRUE(scheduler.IsScheduled(&command1) || + scheduler.IsScheduled(&command2)); + + scheduler.Run(); + EXPECT_EQ(*counterPtr, 2); +} + TEST_F(CommandScheduleTest, SmartDashboardCancel) { CommandScheduler scheduler = GetScheduler(); frc::SmartDashboard::PutData("Scheduler", &scheduler); From b4db87c1cc0df3ed5036e7c210b559b0b0d8b026 Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Fri, 11 Oct 2024 17:33:40 +0300 Subject: [PATCH 4/9] add better error messages to C++ tests --- .../test/native/cpp/frc2/command/CommandScheduleTest.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp index 3b5b0a18987..08dfd64c420 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp @@ -133,11 +133,13 @@ TEST_F(CommandScheduleTest, CancelNextCommand) { // only one of the commands should be canceled. EXPECT_FALSE(scheduler.IsScheduled(&command1) && - scheduler.IsScheduled(&command2)); + scheduler.IsScheduled(&command2)) + << "Both commands are running when only one should be"; // one of the commands shouldn't be canceled because the other one is canceled // first EXPECT_TRUE(scheduler.IsScheduled(&command1) || - scheduler.IsScheduled(&command2)); + scheduler.IsScheduled(&command2)) + << "Both commands are canceled when only one should be"; scheduler.Run(); EXPECT_EQ(*counterPtr, 2); From 4165f52556c2109b2637b4f177831ba62b88ffcf Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Sat, 12 Oct 2024 01:37:38 +0300 Subject: [PATCH 5/9] add a test checking if command is scheduled on end() call --- .../cpp/frc2/command/CommandScheduleTest.cpp | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp index 08dfd64c420..010b269f8b3 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp @@ -129,7 +129,8 @@ TEST_F(CommandScheduleTest, CancelNextCommand) { scheduler.Schedule(&command2); scheduler.Run(); - EXPECT_EQ(*counterPtr, 1); + EXPECT_EQ(*counterPtr, 1) + << "Second command was run when it shouldn't have been"; // only one of the commands should be canceled. EXPECT_FALSE(scheduler.IsScheduled(&command1) && @@ -145,6 +146,26 @@ TEST_F(CommandScheduleTest, CancelNextCommand) { EXPECT_EQ(*counterPtr, 2); } +TEST_F(CommandScheduleTest, CommandKnowsWhenItEnded) { + CommandScheduler scheduler = GetScheduler(); + + frc2::FunctionalCommand* commandPtr = nullptr; + auto command = frc2::FunctionalCommand( + [] {}, [] {}, + [&](auto isForced) { + EXPECT_FALSE(scheduler.IsScheduled(commandPtr)) + << "Command shouldn't be scheduled when its end is called"; + }, + [] { return true; }); + commandPtr = &command; + + scheduler.Schedule(commandPtr); + scheduler.Run(); + EXPECT_FALSE(scheduler.IsScheduled(commandPtr)) + << "Command should be removed from scheduler when its isFinished() " + "returns true"; +} + TEST_F(CommandScheduleTest, SmartDashboardCancel) { CommandScheduler scheduler = GetScheduler(); frc::SmartDashboard::PutData("Scheduler", &scheduler); From cb8c9a1d158be5a720490daba4fc24254091f2d2 Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Sat, 12 Oct 2024 04:12:36 +0300 Subject: [PATCH 6/9] Add test for scheduling a command within a command --- .../cpp/frc2/command/CommandScheduleTest.cpp | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp index 010b269f8b3..e046737b53d 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp @@ -2,6 +2,8 @@ // 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. +#include +#include #include #include @@ -99,13 +101,6 @@ TEST_F(CommandScheduleTest, SchedulerCancel) { EXPECT_FALSE(scheduler.IsScheduled(&command)); } -TEST_F(CommandScheduleTest, NotScheduledCancel) { - CommandScheduler scheduler = GetScheduler(); - MockCommand command; - - EXPECT_NO_FATAL_FAILURE(scheduler.Cancel(&command)); -} - TEST_F(CommandScheduleTest, CancelNextCommand) { CommandScheduler scheduler = GetScheduler(); @@ -166,6 +161,39 @@ TEST_F(CommandScheduleTest, CommandKnowsWhenItEnded) { "returns true"; } +TEST_F(CommandScheduleTest, ScheduleCommandInCommand) { + CommandScheduler scheduler = GetScheduler(); + int counter = 0; + frc2::InstantCommand* commandPtr = nullptr; + + auto command = frc2::RunCommand([&] { + scheduler.Schedule(commandPtr); + EXPECT_EQ(counter, 1) + << "Command 2's init was not run immediately after getting scheduled"; + }); + auto commandToGetScheduled = frc2::InstantCommand([&counter] { counter++; }); + + commandPtr = &commandToGetScheduled; + + scheduler.Schedule(&command); + scheduler.Run(); + EXPECT_EQ(counter, 1) << "Command 2 was not run when it should have been"; + EXPECT_TRUE(scheduler.IsScheduled(commandPtr)) + << "Command 2 was not added to scheduler"; + + scheduler.Run(); + EXPECT_EQ(counter, 1) << "Command 2 was run when it shouldn't have been"; + EXPECT_FALSE(scheduler.IsScheduled(commandPtr)) + << "Command 2 did not end when it should have"; +} + +TEST_F(CommandScheduleTest, NotScheduledCancel) { + CommandScheduler scheduler = GetScheduler(); + MockCommand command; + + EXPECT_NO_FATAL_FAILURE(scheduler.Cancel(&command)); +} + TEST_F(CommandScheduleTest, SmartDashboardCancel) { CommandScheduler scheduler = GetScheduler(); frc::SmartDashboard::PutData("Scheduler", &scheduler); From 3c185deccfe30286d584c1d3b7ba7b11cb613781 Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Sat, 12 Oct 2024 23:46:44 +0300 Subject: [PATCH 7/9] Refactor tests according to reviews Co-Authored-By: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com> --- .../cpp/frc2/command/CommandScheduleTest.cpp | 48 +++++++++---------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp index e046737b53d..b7d46869530 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp @@ -104,28 +104,27 @@ TEST_F(CommandScheduleTest, SchedulerCancel) { TEST_F(CommandScheduleTest, CancelNextCommand) { CommandScheduler scheduler = GetScheduler(); - frc2::RunCommand* command1PtrPtr = nullptr; - frc2::RunCommand* command2PtrPtr = nullptr; - auto counterPtr = std::make_shared(0); + frc2::RunCommand* command1Ptr = nullptr; + frc2::RunCommand* command2Ptr = nullptr; + int counter = 0; - auto command1 = frc2::RunCommand([counterPtr, &command2PtrPtr, &scheduler] { - scheduler.Cancel(command2PtrPtr); - (*counterPtr)++; + auto command1 = frc2::RunCommand([&counter, &command2Ptr, &scheduler] { + scheduler.Cancel(command2Ptr); + counter++; }); - auto command2 = frc2::RunCommand([counterPtr, &command1PtrPtr, &scheduler] { - scheduler.Cancel(command1PtrPtr); - (*counterPtr)++; + auto command2 = frc2::RunCommand([&counter, &command1Ptr, &scheduler] { + scheduler.Cancel(command1Ptr); + counter++; }); - command1PtrPtr = &command1; - command2PtrPtr = &command2; + command1Ptr = &command1; + command2Ptr = &command2; scheduler.Schedule(&command1); scheduler.Schedule(&command2); scheduler.Run(); - EXPECT_EQ(*counterPtr, 1) - << "Second command was run when it shouldn't have been"; + EXPECT_EQ(counter, 1) << "Second command was run when it shouldn't have been"; // only one of the commands should be canceled. EXPECT_FALSE(scheduler.IsScheduled(&command1) && @@ -138,7 +137,7 @@ TEST_F(CommandScheduleTest, CancelNextCommand) { << "Both commands are canceled when only one should be"; scheduler.Run(); - EXPECT_EQ(*counterPtr, 2); + EXPECT_EQ(counter, 2); } TEST_F(CommandScheduleTest, CommandKnowsWhenItEnded) { @@ -164,26 +163,25 @@ TEST_F(CommandScheduleTest, CommandKnowsWhenItEnded) { TEST_F(CommandScheduleTest, ScheduleCommandInCommand) { CommandScheduler scheduler = GetScheduler(); int counter = 0; - frc2::InstantCommand* commandPtr = nullptr; - - auto command = frc2::RunCommand([&] { - scheduler.Schedule(commandPtr); - EXPECT_EQ(counter, 1) - << "Command 2's init was not run immediately after getting scheduled"; - }); - auto commandToGetScheduled = frc2::InstantCommand([&counter] { counter++; }); + frc2::InstantCommand commandToGetScheduled{[&counter] { counter++; }}; - commandPtr = &commandToGetScheduled; + auto command = + frc2::RunCommand([&counter, &scheduler, &commandToGetScheduled] { + scheduler.Schedule(&commandToGetScheduled); + EXPECT_EQ(counter, 1) + << "Scheduled cmmand's init was not run immediately " + "after getting scheduled"; + }); scheduler.Schedule(&command); scheduler.Run(); EXPECT_EQ(counter, 1) << "Command 2 was not run when it should have been"; - EXPECT_TRUE(scheduler.IsScheduled(commandPtr)) + EXPECT_TRUE(scheduler.IsScheduled(&commandToGetScheduled)) << "Command 2 was not added to scheduler"; scheduler.Run(); EXPECT_EQ(counter, 1) << "Command 2 was run when it shouldn't have been"; - EXPECT_FALSE(scheduler.IsScheduled(commandPtr)) + EXPECT_FALSE(scheduler.IsScheduled(&commandToGetScheduled)) << "Command 2 did not end when it should have"; } From 3078be4dfbe2311e78518ca01ac042f110e433e9 Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Thu, 31 Oct 2024 16:40:30 +0300 Subject: [PATCH 8/9] move some tests to `schedulingRecursionTest` & improve comments --- .../cpp/frc2/command/CommandScheduleTest.cpp | 41 +------------------ .../frc2/command/SchedulingRecursionTest.cpp | 39 ++++++++++++++++++ 2 files changed, 40 insertions(+), 40 deletions(-) diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp index b7d46869530..675e21342b6 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp @@ -101,45 +101,6 @@ TEST_F(CommandScheduleTest, SchedulerCancel) { EXPECT_FALSE(scheduler.IsScheduled(&command)); } -TEST_F(CommandScheduleTest, CancelNextCommand) { - CommandScheduler scheduler = GetScheduler(); - - frc2::RunCommand* command1Ptr = nullptr; - frc2::RunCommand* command2Ptr = nullptr; - int counter = 0; - - auto command1 = frc2::RunCommand([&counter, &command2Ptr, &scheduler] { - scheduler.Cancel(command2Ptr); - counter++; - }); - auto command2 = frc2::RunCommand([&counter, &command1Ptr, &scheduler] { - scheduler.Cancel(command1Ptr); - counter++; - }); - - command1Ptr = &command1; - command2Ptr = &command2; - - scheduler.Schedule(&command1); - scheduler.Schedule(&command2); - scheduler.Run(); - - EXPECT_EQ(counter, 1) << "Second command was run when it shouldn't have been"; - - // only one of the commands should be canceled. - EXPECT_FALSE(scheduler.IsScheduled(&command1) && - scheduler.IsScheduled(&command2)) - << "Both commands are running when only one should be"; - // one of the commands shouldn't be canceled because the other one is canceled - // first - EXPECT_TRUE(scheduler.IsScheduled(&command1) || - scheduler.IsScheduled(&command2)) - << "Both commands are canceled when only one should be"; - - scheduler.Run(); - EXPECT_EQ(counter, 2); -} - TEST_F(CommandScheduleTest, CommandKnowsWhenItEnded) { CommandScheduler scheduler = GetScheduler(); @@ -169,7 +130,7 @@ TEST_F(CommandScheduleTest, ScheduleCommandInCommand) { frc2::RunCommand([&counter, &scheduler, &commandToGetScheduled] { scheduler.Schedule(&commandToGetScheduled); EXPECT_EQ(counter, 1) - << "Scheduled cmmand's init was not run immediately " + << "Scheduled command's init was not run immediately " "after getting scheduled"; }); diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/SchedulingRecursionTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/SchedulingRecursionTest.cpp index 52436b97ac6..0b54c589515 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/SchedulingRecursionTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/SchedulingRecursionTest.cpp @@ -339,6 +339,45 @@ TEST_F(SchedulingRecursionTest, CancelDefaultCommandFromEnd) { EXPECT_TRUE(scheduler.IsScheduled(&other)); } +TEST_F(SchedulingRecursionTest, CancelNextCommandFromCommand) { + CommandScheduler scheduler = GetScheduler(); + + frc2::RunCommand* command1Ptr = nullptr; + frc2::RunCommand* command2Ptr = nullptr; + int counter = 0; + + auto command1 = frc2::RunCommand([&counter, &command2Ptr, &scheduler] { + scheduler.Cancel(command2Ptr); + counter++; + }); + auto command2 = frc2::RunCommand([&counter, &command1Ptr, &scheduler] { + scheduler.Cancel(command1Ptr); + counter++; + }); + + command1Ptr = &command1; + command2Ptr = &command2; + + scheduler.Schedule(&command1); + scheduler.Schedule(&command2); + scheduler.Run(); + + EXPECT_EQ(counter, 1) << "Second command was run when it shouldn't have been"; + + // only one of the commands should be canceled. + EXPECT_FALSE(scheduler.IsScheduled(&command1) && + scheduler.IsScheduled(&command2)) + << "Both commands are running when only one should be"; + // one of the commands shouldn't be canceled because the other one is canceled + // first + EXPECT_TRUE(scheduler.IsScheduled(&command1) || + scheduler.IsScheduled(&command2)) + << "Both commands are canceled when only one should be"; + + scheduler.Run(); + EXPECT_EQ(counter, 2); +} + INSTANTIATE_TEST_SUITE_P( SchedulingRecursionTests, SchedulingRecursionTest, testing::Values(Command::InterruptionBehavior::kCancelSelf, From 94c5226c7c247e555533f66c307f84d27a169790 Mon Sep 17 00:00:00 2001 From: Kaya <95276965+kytpbs@users.noreply.github.com> Date: Tue, 17 Dec 2024 22:25:57 +0100 Subject: [PATCH 9/9] Remove Unnecessary parts + correct imports Co-Authored-By: Ryan Blue <13878527+rzblue@users.noreply.github.com> --- .../main/native/cpp/frc2/command/CommandScheduler.cpp | 1 - .../test/native/cpp/frc2/command/CommandScheduleTest.cpp | 9 +++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp index 0a80572e299..369e14367bc 100644 --- a/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp +++ b/wpilibNewCommands/src/main/native/cpp/frc2/command/CommandScheduler.cpp @@ -225,7 +225,6 @@ void CommandScheduler::Run() { } m_watchdog.AddEpoch(command->GetName() + ".End(false)"); - // remove owned commands after everything else is done m_impl->ownedCommands.erase(command); } diff --git a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp index 675e21342b6..840c05bb213 100644 --- a/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp +++ b/wpilibNewCommands/src/test/native/cpp/frc2/command/CommandScheduleTest.cpp @@ -2,16 +2,13 @@ // 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. -#include -#include -#include - -#include - #include #include #include "CommandTestBase.h" +#include "frc2/command/FunctionalCommand.h" +#include "frc2/command/InstantCommand.h" +#include "frc2/command/RunCommand.h" using namespace frc2; class CommandScheduleTest : public CommandTestBase {};