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.

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

Add command.repeatdly(int count) to both java and c++ #6589

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,26 @@ public RepeatCommand repeatedly() {
return new RepeatCommand(this);
}

/**
* Decorates this command to run repeatedly, restarting until the command runs for the given
* number of times. The decorated command can still be canceled.
*
* <p>Note: This decorator works by adding this command to a composition. The command the
* decorator was called on cannot be scheduled independently or be added to a different
* composition (namely, decorators), unless it is manually cleared from the list of composed
* commands with {@link CommandScheduler#removeComposedCommand(Command)}. The command composition
* returned from this method can be further decorated without issue.
*
* @param repetitions the count of times to run the command.
* @return the decorated command
*/
public ParallelRaceGroup repeatedly(int repetitions) {
// Use an array so that it stays in the heap instead of stack.
// We use an array with a size of 1 instead of `AtomicInteger` because of performance difference
int[] counter = {0};
kytpbs marked this conversation as resolved.
Show resolved Hide resolved
return this.finallyDo(() -> counter[0]++).repeatedly().until(() -> counter[0] >= repetitions);
}

/**
* Decorates this command to run "by proxy" by wrapping it in a {@link ProxyCommand}. Use this for
* "forking off" from command compositions when the user does not wish to extend the command's
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ CommandPtr Command::Repeatedly() && {
return std::move(*this).ToPtr().Repeatedly();
}

CommandPtr Command::Repeatedly(int times) && {
return std::move(*this).ToPtr().Repeatedly(times);
}

CommandPtr Command::AsProxy() && {
return std::move(*this).ToPtr().AsProxy();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ CommandPtr CommandPtr::Repeatedly() && {
return std::move(*this);
}

CommandPtr CommandPtr::Repeatedly(int times) && {
AssertValid();
std::shared_ptr<int> countPtr = std::make_shared<int>(0);
return std::move(*this)
.FinallyDo([countPtr] { (*countPtr)++; })
.Repeatedly()
.Until([countPtr, times] { return ((*countPtr) >= times); });
}

CommandPtr CommandPtr::AsProxy() && {
AssertValid();
m_ptr = std::make_unique<ProxyCommand>(std::move(m_ptr));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,16 @@ class Command : public wpi::Sendable, public wpi::SendableHelper<Command> {
[[nodiscard]]
CommandPtr Repeatedly() &&;

/**
* Decorates this command to run repeatedly until the given count is reached
* or is interrupted. The decorated command can still be canceled.
rzblue marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +273 to +274
Copy link
Contributor

Choose a reason for hiding this comment

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

We should either make Java match C++ or C++ match Java. (I don't have a particularly strong preference on which one)
This also applies to the CommandPtr doc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would mean we would neet to also change the documentation on the .repeating() function.

IMO, it would be better if this PR didn't have any line removals

Copy link
Contributor

Choose a reason for hiding this comment

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

This would mean we would neet to also change the documentation on the .repeating() function.

How so? I'm just talking about changing the repeatedly(int) doc comments.

Also, the wording that would most match the repeatedly() doc comment (in both Java and C++) is "Decorates this command to run repeatedly, restarting it when it ends, until this command is run the specified number of times or is interrupted. The decorated command can still be canceled."

*
* @param times the number/count of times to run the command
* @return the decorated command
*/
[[nodiscard]]
CommandPtr Repeatedly(int times) &&;

/**
* Decorates this command to run "by proxy" by wrapping it in a ProxyCommand.
* Use this for "forking off" from command compositions when the user does not
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ class CommandPtr final {
[[nodiscard]]
CommandPtr Repeatedly() &&;

/**
* Decorates this command to run repeatedly, restarting until the command runs
* for the given number of times. The decorated command can still be canceled.
*
* @param times the number of times to run the command
* @return the decorated command
*/
[[nodiscard]]
CommandPtr Repeatedly(int times) &&;

/**
* Decorates this command to run "by proxy" by wrapping it in a ProxyCommand.
* Use this for "forking off" from command compositions when the user does not
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,48 @@ void raceWithOrderTest() {
}
}

@Test
void repeatedlyTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {
AtomicInteger counter = new AtomicInteger(0);

Command command = new InstantCommand(counter::incrementAndGet);

Command group = command.repeatedly();

scheduler.schedule(group);
for (int i = 1; i <= 50; i++) {
scheduler.run();
assertEquals(i, counter.get());
}

// Should still be scheduled
assertTrue(scheduler.isScheduled(group));
}
}

@Test
void repeatedlyCountTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {
AtomicInteger counter = new AtomicInteger(0);

Command command = new InstantCommand(counter::incrementAndGet);

Command group = command.repeatedly(3);

scheduler.schedule(group);
assertEquals(1, counter.get());
for (int i = 1; i < 3; i++) {
scheduler.run();
assertEquals(i, counter.get());
assertTrue(scheduler.isScheduled(group), "Expected group to be scheduled with i = " + i);
}
scheduler.run();
assertEquals(3, counter.get(), "Loop should have run 3 times something went wrong");
assertFalse(scheduler.isScheduled(group), "This command should have gotten unscheduled");
}
}

@Test
void unlessTest() {
try (CommandScheduler scheduler = new CommandScheduler()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,45 @@ TEST_F(CommandDecoratorTest, RaceWithOrder) {
EXPECT_TRUE(firstWasPolled);
}

TEST_F(CommandDecoratorTest, Repeatedly) {
CommandScheduler scheduler = GetScheduler();

int counter = 0;

auto command = InstantCommand([&counter] { counter++; }, {}).Repeatedly();

scheduler.Schedule(command);

for (int i = 1; i <= 50; i++) {
scheduler.Run();
EXPECT_EQ(i, counter);
}

EXPECT_TRUE(scheduler.IsScheduled(command));
}

TEST_F(CommandDecoratorTest, RepeatedlyCount) {
CommandScheduler scheduler = GetScheduler();

int counter = 0;

auto command = InstantCommand([&counter] { counter++; }, {}).Repeatedly(3);

scheduler.Schedule(command);
EXPECT_TRUE(scheduler.IsScheduled(command));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
EXPECT_TRUE(scheduler.IsScheduled(command));
EXPECT_EQ(1, counter);

This matches Java and is more useful than the current check.
There's also a few more small changes- Make sure to read the whole comment.

for (int i = 1; i < 3; i++) {
scheduler.Run();
EXPECT_EQ(i, counter);
kytpbs marked this conversation as resolved.
Show resolved Hide resolved
EXPECT_TRUE(scheduler.IsScheduled(command))
<< "Expected group to be scheduled with i = " << i;
}

scheduler.Run();
EXPECT_EQ(3, counter) << "Loop should have run 3 times something went wrong";
EXPECT_FALSE(scheduler.IsScheduled(command))
<< "This command should have gotten unscheduled";
}

TEST_F(CommandDecoratorTest, Unless) {
CommandScheduler scheduler = GetScheduler();

Expand Down
Loading