Skip to content

[commands v3] Make Mechanism an interface #8303

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: 2027
Choose a base branch
from
Open

[commands v3] Make Mechanism an interface #8303

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43971b5
made Mechanism an interface
Daniel1464 Oct 26, 2025
cec5d64
Change Mechanism.createDummy() to DummyMechanism
Daniel1464 Oct 26, 2025
97dc7f1
Fixed tests and moved DummyMechanism to test folder
Daniel1464 Oct 26, 2025
81df575
Docs improvements
Daniel1464 Oct 26, 2025
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
75 changes: 22 additions & 53 deletions commandsv3/src/main/java/org/wpilib/commands3/Mechanism.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,51 +10,25 @@
import org.wpilib.annotation.NoDiscard;

/**
* Generic base class to represent mechanisms on a robot. Commands can require sole ownership of a
* Generic interface to represent mechanisms on a robot. Commands can require sole ownership of a
* mechanism; when a command that requires a mechanism is running, no other commands may use it at
* the same time.
*
* <p>Even though this class is named "Mechanism", it may be used to represent other physical
* <p>Unlike the Subsystem interface of commands v2, Mechanism does not need to be explicitly
* registered in the constructor.
*
* <p>Even though this interface is named "Mechanism", it may be used to represent other physical
* hardware on a robot that should be controlled with commands - for example, an LED strip or a
* vision processor that can switch between different pipelines could be represented as mechanisms.
*/
public class Mechanism {
private final String m_name;

private final Scheduler m_registeredScheduler;

/**
* Creates a new mechanism registered with the default scheduler instance and named using the name
* of the class. Intended to be used by subclasses to get sane defaults without needing to
* manually declare a constructor.
*/
@SuppressWarnings("this-escape")
protected Mechanism() {
m_name = getClass().getSimpleName();
m_registeredScheduler = Scheduler.getDefault();
setDefaultCommand(idle());
}

public interface Mechanism {
/**
* Creates a new mechanism, registered with the default scheduler instance.
* Gets the scheduler that registered this subsystem.
*
* @param name The name of the mechanism. Cannot be null.
* @return The registered scheduler.
*/
public Mechanism(String name) {
this(name, Scheduler.getDefault());
}

/**
* Creates a new mechanism, registered with the given scheduler instance.
*
* @param name The name of the mechanism. Cannot be null.
* @param scheduler The registered scheduler. Cannot be null.
*/
@SuppressWarnings("this-escape")
public Mechanism(String name, Scheduler scheduler) {
m_name = name;
m_registeredScheduler = scheduler;
setDefaultCommand(idle());
default Scheduler getRegisteredScheduler() {
return Scheduler.getDefault();
}

/**
Expand All @@ -63,8 +37,8 @@ public Mechanism(String name, Scheduler scheduler) {
* @return The name of the mechanism.
*/
@NoDiscard
public String getName() {
return m_name;
default String getName() {
return this.getClass().getSimpleName();
}

/**
Expand All @@ -79,8 +53,8 @@ public String getName() {
*
* @param defaultCommand the new default command
*/
public void setDefaultCommand(Command defaultCommand) {
m_registeredScheduler.setDefaultCommand(this, defaultCommand);
default void setDefaultCommand(Command defaultCommand) {
getRegisteredScheduler().setDefaultCommand(this, defaultCommand);
}

/**
Expand All @@ -89,8 +63,8 @@ public void setDefaultCommand(Command defaultCommand) {
*
* @return The currently configured default command
*/
public Command getDefaultCommand() {
return m_registeredScheduler.getDefaultCommandFor(this);
default Command getDefaultCommand() {
return getRegisteredScheduler().getDefaultCommandFor(this);
}

/**
Expand All @@ -99,7 +73,7 @@ public Command getDefaultCommand() {
* @param commandBody The main function body of the command.
* @return The command builder, for further configuration.
*/
public NeedsNameBuilderStage run(Consumer<Coroutine> commandBody) {
default NeedsNameBuilderStage run(Consumer<Coroutine> commandBody) {
return new StagedCommandBuilder().requiring(this).executing(commandBody);
}

Expand All @@ -111,7 +85,7 @@ public NeedsNameBuilderStage run(Consumer<Coroutine> commandBody) {
* @param loopBody The body of the infinite loop.
* @return The command builder, for further configuration.
*/
public NeedsNameBuilderStage runRepeatedly(Runnable loopBody) {
default NeedsNameBuilderStage runRepeatedly(Runnable loopBody) {
return run(
coroutine -> {
while (true) {
Expand All @@ -131,7 +105,7 @@ public NeedsNameBuilderStage runRepeatedly(Runnable loopBody) {
*
* @return A new idle command.
*/
public Command idle() {
default Command idle() {
return run(Coroutine::park).withPriority(Command.LOWEST_PRIORITY).named(getName() + "[IDLE]");
}

Expand All @@ -141,7 +115,7 @@ public Command idle() {
* @param duration How long the mechanism should idle for.
* @return A new idle command.
*/
public Command idleFor(Time duration) {
default Command idleFor(Time duration) {
return idle().withTimeout(duration);
}

Expand All @@ -154,12 +128,7 @@ public Command idleFor(Time duration) {
* @return The currently running commands that require the mechanism.
*/
@NoDiscard
public List<Command> getRunningCommands() {
return m_registeredScheduler.getRunningCommandsFor(this);
}

@Override
public String toString() {
Copy link
Contributor

@KangarooKoala KangarooKoala Oct 26, 2025

Choose a reason for hiding this comment

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

Any concerns about losing this default toString()? I'm not familiar enough with commands v3 to know if people may print a Mechanism or a collection of Mechanisms. (e.g., someone prints cmd.requirements() when debugging)
There also isn't much of a difference because the default toString() also contains the class's name, but the @xxxxxx might be confusing.
It's not a huge deal, but I just want to double check.

Copy link
Contributor

@DeltaDizzy DeltaDizzy Oct 26, 2025

Choose a reason for hiding this comment

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

I had initially (when the idea to convert first came up) suggested a split to Mechanism (interface) and MechanismBase a la v2 for this reason, though I don't feel super strongly about it (and in fact don't really like "MechanismBase" as a name).

In practice I don't think many teams give their subsystems names different from their classes, but it could show up in the case of having multiple copies of a subsystem on the robot (which I actually have done before!). If it is deemed an uncommon case it is probably fine to just tell teams to manually override it in their subsystem if they want to change the string representation (maybe include that in the Mechanism template used in the project generator?).

Copy link
Contributor

@KangarooKoala KangarooKoala Oct 27, 2025

Choose a reason for hiding this comment

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

I agree that teams wanting subsystems to have names other than the class names can override getName() (even with the slight amount of plumbing to have separate names for difference instances).

My main concern is about the default toString() including the @xxxxxx stuff at the end, which could be confusing for rookies. Again, it probably isn't an issue since they probably won't see that toString() output, but I want to make sure that we have proper justification.

Copy link
Member

@SamCarlberg SamCarlberg Oct 27, 2025

Choose a reason for hiding this comment

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

getName() is used in telemetry, but many inexperienced programmers will just print objects to System.out.println for quick and dirty debugging, and that will use the default toString() implementations. I don't think that's generally an issue, since most subsystems only have a single instance, and those that have multiple should override getName() anyway to have unique names for each instance

Copy link
Contributor

@KangarooKoala KangarooKoala Oct 27, 2025

Choose a reason for hiding this comment

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

If the only way people will see toString() is from directly printing the Mechanism objects (or a collection of Mechanism objects), then yeah it's probably fine. (Just wanted to double-check that there wasn't any other way people might see the toString().)

Copy link
Contributor Author

@Daniel1464 Daniel1464 Oct 29, 2025

Choose a reason for hiding this comment

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

yeah, the idea was to let ppl override getName() instead.

return m_name;
default List<Command> getRunningCommands() {
return getRegisteredScheduler().getRunningCommandsFor(this);
}
}
8 changes: 4 additions & 4 deletions commandsv3/src/test/java/org/wpilib/commands3/ConflictDetectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,24 +23,24 @@ void emptyInputHasNoConflicts() {

@Test
void singleInputHasNoConflicts() {
var mech = new Mechanism("Mech", m_scheduler);
var mech = new DummyMechanism("Mech", m_scheduler);
var command = Command.requiring(mech).executing(Coroutine::park).named("Command");
var conflicts = findAllConflicts(List.of(command));
assertEquals(0, conflicts.size());
}

@Test
void commandDoesNotConflictWithSelf() {
var mech = new Mechanism("Mech", m_scheduler);
var mech = new DummyMechanism("Mech", m_scheduler);
var command = Command.requiring(mech).executing(Coroutine::park).named("Command");
var conflicts = findAllConflicts(List.of(command, command));
assertEquals(0, conflicts.size());
}

@Test
void detectManyConflicts() {
var mech1 = new Mechanism("Mech 1", m_scheduler);
var mech2 = new Mechanism("Mech 2", m_scheduler);
var mech1 = new DummyMechanism("Mech 1", m_scheduler);
var mech2 = new DummyMechanism("Mech 2", m_scheduler);

var command1 = Command.requiring(mech1, mech2).executing(Coroutine::park).named("Command1");
var command2 = Command.requiring(mech1).executing(Coroutine::park).named("Command2");
Expand Down
32 changes: 32 additions & 0 deletions commandsv3/src/test/java/org/wpilib/commands3/DummyMechanism.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// 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 org.wpilib.commands3;

/** A dummy mechanism that allows inline scheduler and name specification, for use in unit tests. */
class DummyMechanism implements Mechanism {
private final String m_name;
private final Scheduler m_scheduler;

/**
* Creates a dummy mechanism.
*
* @param name The name of this dummy mechanism.
* @param scheduler The registered scheduler. Cannot be null.
*/
DummyMechanism(String name, Scheduler scheduler) {
m_name = name;
m_scheduler = scheduler;
}

@Override
public String getName() {
return m_name;
}

@Override
public Scheduler getRegisteredScheduler() {
return m_scheduler;
}
}
18 changes: 9 additions & 9 deletions commandsv3/src/test/java/org/wpilib/commands3/ParallelGroupTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
class ParallelGroupTest extends CommandTestBase {
@Test
void parallelAll() {
var r1 = new Mechanism("R1", m_scheduler);
var r2 = new Mechanism("R2", m_scheduler);
var r1 = new DummyMechanism("R1", m_scheduler);
var r2 = new DummyMechanism("R2", m_scheduler);

var c1Count = new AtomicInteger(0);
var c2Count = new AtomicInteger(0);
Expand Down Expand Up @@ -81,8 +81,8 @@ void parallelAll() {

@Test
void race() {
var r1 = new Mechanism("R1", m_scheduler);
var r2 = new Mechanism("R2", m_scheduler);
var r1 = new DummyMechanism("R1", m_scheduler);
var r2 = new DummyMechanism("R2", m_scheduler);

var c1Count = new AtomicInteger(0);
var c2Count = new AtomicInteger(0);
Expand Down Expand Up @@ -134,7 +134,7 @@ void race() {

@Test
void nested() {
var mechanism = new Mechanism("mechanism", m_scheduler);
var mechanism = new DummyMechanism("mechanism", m_scheduler);

var count = new AtomicInteger(0);

Expand Down Expand Up @@ -213,8 +213,8 @@ void automaticNameDeadline() {

@Test
void inheritsRequirements() {
var mech1 = new Mechanism("Mech 1", m_scheduler);
var mech2 = new Mechanism("Mech 2", m_scheduler);
var mech1 = new DummyMechanism("Mech 1", m_scheduler);
var mech2 = new DummyMechanism("Mech 2", m_scheduler);
var command1 = mech1.run(Coroutine::park).named("Command 1");
var command2 = mech2.run(Coroutine::park).named("Command 2");
var group = new ParallelGroup("Group", Set.of(command1, command2), Set.of());
Expand All @@ -223,8 +223,8 @@ void inheritsRequirements() {

@Test
void inheritsPriority() {
var mech1 = new Mechanism("Mech 1", m_scheduler);
var mech2 = new Mechanism("Mech 2", m_scheduler);
var mech1 = new DummyMechanism("Mech 1", m_scheduler);
var mech2 = new DummyMechanism("Mech 2", m_scheduler);
var command1 = mech1.run(Coroutine::park).withPriority(100).named("Command 1");
var command2 = mech2.run(Coroutine::park).withPriority(200).named("Command 2");
var group = new ParallelGroup("Group", Set.of(command1, command2), Set.of());
Expand Down
24 changes: 13 additions & 11 deletions commandsv3/src/test/java/org/wpilib/commands3/SchedulerCancellationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ class SchedulerCancellationTests extends CommandTestBase {
void cancelOnInterruptDoesNotResume() {
var count = new AtomicInteger(0);

var mechanism = new Mechanism("mechanism", m_scheduler);
var mechanism = new DummyMechanism("mechanism", m_scheduler);

var interrupter =
Command.requiring(mechanism)
Expand Down Expand Up @@ -54,7 +54,7 @@ void cancelOnInterruptDoesNotResume() {
void defaultCommandResumesAfterInterruption() {
var count = new AtomicInteger(0);

var mechanism = new Mechanism("mechanism", m_scheduler);
var mechanism = new DummyMechanism("mechanism", m_scheduler);
var defaultCmd =
mechanism
.run(
Expand Down Expand Up @@ -169,7 +169,9 @@ void cancelAllDoesNotCallOnCancelHookForQueuedCommands() {
void cancelAllStartsDefaults() {
var mechanisms = new ArrayList<Mechanism>(10);
for (int i = 1; i <= 10; i++) {
mechanisms.add(new Mechanism("System " + i, m_scheduler));
var mechanism = new DummyMechanism("System " + i, m_scheduler);
mechanism.setDefaultCommand(mechanism.idle());
Copy link
Contributor

@KangarooKoala KangarooKoala Oct 26, 2025

Choose a reason for hiding this comment

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

Why not put a setDefaultCommand(idle()); into the DummyMechanism constructor?

Copy link
Contributor Author

@Daniel1464 Daniel1464 Oct 27, 2025

Choose a reason for hiding this comment

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

Since the default command of Mechanism is now null instead of idle(), and since DummyMechanism is just supposed to be a Mechanism itself, I don't think having it implicitly do this is the best. I could be wrong tho

Copy link
Contributor

@KangarooKoala KangarooKoala Oct 27, 2025

Choose a reason for hiding this comment

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

👍 Yeah, that seems fair. I guess another option would be to have a decorator that sets the default command:

public DummyMechanism withIdleDefaultCommand() {
  setDefaultCommand(idle());
  return this;
}

// Then we can do e.g. mechanisms.add(new DummyMechanism("System " + i, m_scheduler).withIdleDefaultCommand());

This is just for illustrative purposes- Exact signature (return type and name) can vary.
This also might be overkill, anyways- Calling setDefaultCommand() for the different instances is annoying but ultimately not that bad. I'm fine with whatever you decide (which could just be marking this as resolved).

mechanisms.add(mechanism);
}

var command = Command.requiring(mechanisms).executing(Coroutine::yield).named("Big Command");
Expand Down Expand Up @@ -237,7 +239,7 @@ void cancelDeeplyNestedCompositions() {

@Test
void compositionsDoNotSelfCancel() {
var mech = new Mechanism("The mechanism", m_scheduler);
var mech = new DummyMechanism("The mechanism", m_scheduler);
var group =
mech.run(
co -> {
Expand All @@ -263,7 +265,7 @@ void compositionsDoNotSelfCancel() {

@Test
void compositionsDoNotCancelParent() {
var mech = new Mechanism("The mechanism", m_scheduler);
var mech = new DummyMechanism("The mechanism", m_scheduler);
var group =
mech.run(
co -> {
Expand All @@ -286,7 +288,7 @@ void compositionsDoNotCancelParent() {
void doesNotRunOnCancelWhenInterruptingOnDeck() {
var ran = new AtomicBoolean(false);

var mechanism = new Mechanism("The mechanism", m_scheduler);
var mechanism = new DummyMechanism("The mechanism", m_scheduler);
var cmd = mechanism.run(Coroutine::yield).whenCanceled(() -> ran.set(true)).named("cmd");
var interrupter = mechanism.run(Coroutine::yield).named("Interrupter");
m_scheduler.schedule(cmd);
Expand All @@ -300,7 +302,7 @@ void doesNotRunOnCancelWhenInterruptingOnDeck() {
void doesNotRunOnCancelWhenCancelingOnDeck() {
var ran = new AtomicBoolean(false);

var mechanism = new Mechanism("The mechanism", m_scheduler);
var mechanism = new DummyMechanism("The mechanism", m_scheduler);
var cmd = mechanism.run(Coroutine::yield).whenCanceled(() -> ran.set(true)).named("cmd");
m_scheduler.schedule(cmd);
// canceling before calling .run()
Expand All @@ -314,7 +316,7 @@ void doesNotRunOnCancelWhenCancelingOnDeck() {
void runsOnCancelWhenInterruptingCommand() {
var ran = new AtomicBoolean(false);

var mechanism = new Mechanism("The mechanism", m_scheduler);
var mechanism = new DummyMechanism("The mechanism", m_scheduler);
var cmd = mechanism.run(Coroutine::park).whenCanceled(() -> ran.set(true)).named("cmd");
var interrupter = mechanism.run(Coroutine::park).named("Interrupter");
m_scheduler.schedule(cmd);
Expand All @@ -329,7 +331,7 @@ void runsOnCancelWhenInterruptingCommand() {
void doesNotRunOnCancelWhenCompleting() {
var ran = new AtomicBoolean(false);

var mechanism = new Mechanism("The mechanism", m_scheduler);
var mechanism = new DummyMechanism("The mechanism", m_scheduler);
var cmd = mechanism.run(Coroutine::yield).whenCanceled(() -> ran.set(true)).named("cmd");
m_scheduler.schedule(cmd);
m_scheduler.run();
Expand All @@ -343,7 +345,7 @@ void doesNotRunOnCancelWhenCompleting() {
void runsOnCancelWhenCanceling() {
var ran = new AtomicBoolean(false);

var mechanism = new Mechanism("The mechanism", m_scheduler);
var mechanism = new DummyMechanism("The mechanism", m_scheduler);
var cmd = mechanism.run(Coroutine::yield).whenCanceled(() -> ran.set(true)).named("cmd");
m_scheduler.schedule(cmd);
m_scheduler.run();
Expand All @@ -356,7 +358,7 @@ void runsOnCancelWhenCanceling() {
void runsOnCancelWhenCancelingParent() {
var ran = new AtomicBoolean(false);

var mechanism = new Mechanism("The mechanism", m_scheduler);
var mechanism = new DummyMechanism("The mechanism", m_scheduler);
var cmd = mechanism.run(Coroutine::yield).whenCanceled(() -> ran.set(true)).named("cmd");

var group = new SequentialGroup("Seq", Collections.singletonList(cmd));
Expand Down
8 changes: 4 additions & 4 deletions commandsv3/src/test/java/org/wpilib/commands3/SchedulerConflictTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
class SchedulerConflictTests extends CommandTestBase {
@Test
void compositionsCannotAwaitConflictingCommands() {
var mech = new Mechanism("The Mechanism", m_scheduler);
var mech = new DummyMechanism("The Mechanism", m_scheduler);

var group =
Command.noRequirements()
Expand All @@ -41,7 +41,7 @@ void compositionsCannotAwaitConflictingCommands() {

@Test
void innerCommandMayInterruptOtherInnerCommand() {
var mechanism = new Mechanism("The mechanism", m_scheduler);
var mechanism = new DummyMechanism("The mechanism", m_scheduler);
var firstRan = new AtomicBoolean(false);
var secondRan = new AtomicBoolean(false);

Expand Down Expand Up @@ -124,7 +124,7 @@ void nestedOneShotCompositionsAllRunInOneCycle() {

@Test
void childConflictsWithHigherPriorityTopLevel() {
var mechanism = new Mechanism("mechanism", m_scheduler);
var mechanism = new DummyMechanism("mechanism", m_scheduler);
var top = mechanism.run(Coroutine::park).withPriority(10).named("Top");

// Child conflicts with and is lower priority than the Top command
Expand All @@ -143,7 +143,7 @@ void childConflictsWithHigherPriorityTopLevel() {

@Test
void childConflictsWithLowerPriorityTopLevel() {
var mechanism = new Mechanism("mechanism", m_scheduler);
var mechanism = new DummyMechanism("mechanism", m_scheduler);
var top = mechanism.run(Coroutine::park).withPriority(-10).named("Top");

// Child conflicts with and is higher priority than the Top command
Expand Down
Loading
Loading