From f5c9a193293c6e326bb0d4004072398ade16ef32 Mon Sep 17 00:00:00 2001 From: Oblarg Date: Tue, 14 May 2024 11:32:24 -0400 Subject: [PATCH] remove loop funcs and default commands from `resource` --- .../wpilibj2/command/CommandScheduler.java | 146 +++++++----------- .../wpi/first/wpilibj2/command/Resource.java | 54 ------- .../wpi/first/wpilibj2/command/Subsystem.java | 69 ++++++++- .../first/wpilibj2/command/SubsystemBase.java | 6 +- .../wpilibj2/command/sysid/SysIdRoutine.java | 1 - .../first/wpilibj2/command/SchedulerTest.java | 8 +- 6 files changed, 129 insertions(+), 155 deletions(-) diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java index c89fcff3229..e69db2a55f3 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/CommandScheduler.java @@ -41,7 +41,7 @@ * The scheduler responsible for running {@link Command}s. A Command-based robot should call {@link * CommandScheduler#run()} on the singleton instance in its periodic block in order to run commands * synchronously from the main loop. Subsystems should be registered with the scheduler using {@link - * CommandScheduler#registerResources(Resource...)} in order for their {@link Resource#periodic()} + * CommandScheduler#registerSubsystem(Subsystem...)} in order for their {@link Subsystem#periodic()} * methods to be called and for their default commands to be scheduled. * *

This class is provided by the NewCommands VendorDep @@ -71,12 +71,12 @@ public static synchronized CommandScheduler getInstance() { // A map from required resources to their requiring commands. Also used as a set of the // currently-required resources. - private final Map m_requirements = new LinkedHashMap<>(); - - // A map from resources registered with the scheduler to their default commands. Also used - // as a list of currently-registered resources. private final Map m_resources = new LinkedHashMap<>(); + // A map from resources subsystems with the scheduler to their default commands. Also used + // as a list of currently-registered subsystems. + private final Map m_subsystems = new LinkedHashMap<>(); + private final EventLoop m_defaultButtonLoop = new EventLoop(); // The set of currently-registered buttons that will be polled every iteration. private EventLoop m_activeButtonLoop = m_defaultButtonLoop; @@ -164,7 +164,7 @@ public void setActiveButtonLoop(EventLoop loop) { private void initCommand(Command command, Set requirements) { m_scheduledCommands.add(command); for (Resource requirement : requirements) { - m_requirements.put(requirement, command); + m_resources.put(requirement, command); } command.initialize(); for (Consumer action : m_initActions) { @@ -205,7 +205,7 @@ private void schedule(Command command) { Set requirements = command.getRequirements(); // Schedule the command if the requirements are not currently in-use. - if (Collections.disjoint(m_requirements.keySet(), requirements)) { + if (Collections.disjoint(m_resources.keySet(), requirements)) { initCommand(command, requirements); } else { // Else check if the requirements that are in use have all have interruptible commands, @@ -259,12 +259,12 @@ public void run() { m_watchdog.reset(); // Run the periodic method of all registered resources. - for (Resource resource : m_resources.keySet()) { - resource.periodic(); + for (Subsystem subsystem : m_subsystems.keySet()) { + subsystem.periodic(); if (RobotBase.isSimulation()) { - resource.simulationPeriodic(); + subsystem.simulationPeriodic(); } - m_watchdog.addEpoch(resource.getName() + ".periodic()"); + m_watchdog.addEpoch(subsystem.getName() + ".periodic()"); } // Cache the active instance to avoid concurrency problems if setActiveLoop() is called from @@ -299,7 +299,7 @@ public void run() { m_endingCommands.remove(command); iterator.remove(); - m_requirements.keySet().removeAll(command.getRequirements()); + m_resources.keySet().removeAll(command.getRequirements()); m_watchdog.addEpoch(command.getName() + ".end(false)"); } } @@ -319,10 +319,10 @@ public void run() { m_toCancelInterruptors.clear(); // Add default commands for un-required registered resources. - for (Map.Entry resourceCommand : m_resources.entrySet()) { - if (!m_requirements.containsKey(resourceCommand.getKey()) - && resourceCommand.getValue() != null) { - schedule(resourceCommand.getValue()); + for (Map.Entry subsystemCommand : m_subsystems.entrySet()) { + if (!m_resources.containsKey(subsystemCommand.getKey()) + && subsystemCommand.getValue() != null) { + schedule(subsystemCommand.getValue()); } } @@ -334,86 +334,58 @@ public void run() { } /** - * Registers resources with the scheduler. This must be called for the resource's periodic block - * to run when the scheduler is run, and for the resource's default command to be scheduled. It - * is recommended to call this from the constructor of your resource implementations. + * Registers subsystems with the scheduler. This must be called for the subsystem's periodic block + * to run when the scheduler is run, and for the subsystem's default command to be scheduled. It + * is recommended to call this from the constructor of your subsystem implementations. * - * @param resources the resources to register + * @param subsystems the subsystems to register */ - public void registerResources(Resource... resources) { - for (Resource resource : resources) { - if (resource == null) { - DriverStation.reportWarning("Tried to register a null resource", true); + public void registerSubsystem(Subsystem... subsystems) { + for (Subsystem subsystem : subsystems) { + if (subsystem == null) { + DriverStation.reportWarning("Tried to register a null subsystem", true); continue; } - if (m_resources.containsKey(resource)) { - DriverStation.reportWarning("Tried to register an already-registered resource", true); + if (m_subsystems.containsKey(subsystem)) { + DriverStation.reportWarning("Tried to register an already-registered subsystem", true); continue; } - m_resources.put(resource, null); + m_subsystems.put(subsystem, null); } } /** - * Alias of {@link CommandScheduler#registerResources(Resource...)}. See {@link Subsystem} for - * details regarding the name change. - * - * @param subsystems the subsystem(s to register - */ - public void registerSubsystem(Subsystem... subsystems) { - registerResources(subsystems); - } - - /** - * Un-registers resources with the scheduler. The resources will no longer have their periodic + * Un-registers subsystems with the scheduler. The subsystems will no longer have their periodic * block called, and will not have their default command scheduled. * - * @param resources the resources to un-register - */ - public void unregisterResources(Resource... resources) { - m_resources.keySet().removeAll(Set.of(resources)); - } - - /** - * Alias of {@link CommandScheduler#unregisterResources(Resource...)}. See {@link Subsystem} for - * details regarding the name change. - * * @param subsystems the subsystems to un-register */ - public void unregisterSubsystem(Resource... subsystems) { - unregisterResources(subsystems); + public void unregisterSubsystem(Subsystem... subsystems) { + m_subsystems.keySet().removeAll(Set.of(subsystems)); } /** - * Un-registers all registered {@link Resource}s with the scheduler. All currently registered - * resources will no longer have their periodic block called, and will not have their default + * Un-registers all registered {@link Subsystem}s with the scheduler. All currently registered + * subsystems will no longer have their periodic block called, and will not have their default * command scheduled. */ - public void unregisterAllResources() { - m_resources.clear(); - } - - /** - * Alias of {@link CommandScheduler#unregisterAllResources()}. See {@link Subsystem} for - * details regarding the name change. - */ public void unregisterAllSubsystems() { - unregisterAllResources(); + m_subsystems.clear(); } /** - * Sets the default command for a resource. Registers that resource if it is not already + * Sets the default command for a subsystem. Registers that subsystem if it is not already * registered. Default commands will run whenever there is no other command currently scheduled - * that requires the resource. Default commands should be written to never end (i.e. their {@link + * that requires the subsystem. Default commands should be written to never end (i.e. their {@link * Command#isFinished()} method should return false), as they would simply be re-scheduled if they - * do. Default commands must also require their resource. + * do. Default commands must also require their subsystem. * - * @param resource the resource whose default command will be set - * @param defaultCommand the default command to associate with the resource + * @param subsystem the subsystem whose default command will be set + * @param defaultCommand the default command to associate with the subsystem */ - public void setDefaultCommand(Resource resource, Command defaultCommand) { - if (resource == null) { - DriverStation.reportWarning("Tried to set a default command for a null resource", true); + public void setDefaultCommand(Subsystem subsystem, Command defaultCommand) { + if (subsystem == null) { + DriverStation.reportWarning("Tried to set a default command for a null subsystem", true); return; } if (defaultCommand == null) { @@ -423,46 +395,46 @@ public void setDefaultCommand(Resource resource, Command defaultCommand) { requireNotComposed(defaultCommand); - if (!defaultCommand.getRequirements().contains(resource)) { - throw new IllegalArgumentException("Default commands must require their resource!"); + if (!defaultCommand.getRequirements().contains(subsystem)) { + throw new IllegalArgumentException("Default commands must require their subsystem!"); } if (defaultCommand.getInterruptionBehavior() == InterruptionBehavior.kCancelIncoming) { DriverStation.reportWarning( "Registering a non-interruptible default command!\n" - + "This will likely prevent any other commands from requiring this resource.", + + "This will likely prevent any other commands from requiring this subsystem.", true); // Warn, but allow -- there might be a use case for this. } - m_resources.put(resource, defaultCommand); + m_subsystems.put(subsystem, defaultCommand); } /** - * Removes the default command for a resource. The current default command will run until another - * command is scheduled that requires the resource, at which point the current default command + * Removes the default command for a subsystem. The current default command will run until another + * command is scheduled that requires the subsystem, at which point the current default command * will not be re-scheduled. * - * @param resource the resource whose default command will be removed + * @param subsystem the subsystem whose default command will be removed */ - public void removeDefaultCommand(Resource resource) { - if (resource == null) { - DriverStation.reportWarning("Tried to remove a default command for a null resource", true); + public void removeDefaultCommand(Subsystem subsystem) { + if (subsystem == null) { + DriverStation.reportWarning("Tried to remove a default command for a null subsystem", true); return; } - m_resources.put(resource, null); + m_subsystems.put(subsystem, null); } /** - * Gets the default command associated with this resource. Null if this resource has no default + * Gets the default command associated with this subsystem. Null if this subsystem has no default * command associated with it. * - * @param resource the resource to inquire about - * @return the default command associated with the resource + * @param subsystem the subsystem to inquire about + * @return the default command associated with the subsystem */ - public Command getDefaultCommand(Resource resource) { - return m_resources.get(resource); + public Command getDefaultCommand(Subsystem subsystem) { + return m_subsystems.get(subsystem); } /** @@ -514,7 +486,7 @@ private void cancel(Command command, Optional interruptor) { } m_endingCommands.remove(command); m_scheduledCommands.remove(command); - m_requirements.keySet().removeAll(command.getRequirements()); + m_resources.keySet().removeAll(command.getRequirements()); m_watchdog.addEpoch(command.getName() + ".end(true)"); } @@ -545,7 +517,7 @@ public boolean isScheduled(Command... commands) { * scheduled */ public Command requiring(Resource resource) { - return m_requirements.get(resource); + return m_resources.get(resource); } /** Disables the command scheduler. */ diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Resource.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Resource.java index e186df517d2..a1c85187b98 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Resource.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Resource.java @@ -24,21 +24,6 @@ * defined on this interface, which automatically declare the resource requirement. */ public interface Resource { - /** - * This method is called periodically by the {@link CommandScheduler}. Useful for updating - * resource-specific state that you don't want to offload to a {@link Command}. Teams should try - * to be consistent within their own codebases about which responsibilities will be handled by - * Commands, and which will be handled here. - */ - default void periodic() {} - - /** - * This method is called periodically by the {@link CommandScheduler}. Useful for updating - * resource-specific state that needs to be maintained for simulations, such as for updating - * {@link edu.wpi.first.wpilibj.simulation} classes and setting simulated sensor readings. - */ - default void simulationPeriodic() {} - /** * Gets the name of this resource for telemetry. * @@ -48,37 +33,6 @@ default String getName() { return this.getClass().getSimpleName(); } - /** - * Sets the default {@link Command} of the resource. The default command will be automatically - * scheduled when no other commands are scheduled that require the resource. Default commands - * should generally not end on their own, i.e. their {@link Command#isFinished()} method should - * always return false. Will automatically register this resource with the {@link - * CommandScheduler}. - * - * @param defaultCommand the default command to associate with this resource - */ - default void setDefaultCommand(Command defaultCommand) { - CommandScheduler.getInstance().setDefaultCommand(this, defaultCommand); - } - - /** - * Removes the default command for the resource. This will not cancel the default command if it - * is currently running. - */ - default void removeDefaultCommand() { - CommandScheduler.getInstance().removeDefaultCommand(this); - } - - /** - * Gets the default command for this resource. Returns null if no default command is currently - * associated with the resource. - * - * @return the default command associated with this resource - */ - default Command getDefaultCommand() { - return CommandScheduler.getInstance().getDefaultCommand(this); - } - /** * Returns the command currently running on this resource. Returns null if no command is * currently scheduled that requires this resource. @@ -89,14 +43,6 @@ default Command getCurrentCommand() { return CommandScheduler.getInstance().requiring(this); } - /** - * Registers this resource with the {@link CommandScheduler}, allowing its {@link - * Resource#periodic()} method to be called when the scheduler runs. - */ - default void register() { - CommandScheduler.getInstance().registerResources(this); - } - /** * Constructs a command that runs an action once and finishes. Requires this resource. * diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Subsystem.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Subsystem.java index 83936f723f8..346a2ea62cd 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Subsystem.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/Subsystem.java @@ -1,16 +1,12 @@ package edu.wpi.first.wpilibj2.command; /** - * NOTE: The `Subsystem` interface has been renamed to {@link Resource}; this interface remains as - * an alias for backwards-compatibility. The new name better represents the in-code purpose of - * the interface. - * *

A robot subsystem. Subsystems are the basic unit of robot organization in the Command-based * framework; they encapsulate low-level hardware objects (motor controllers, sensors, etc.) and * provide methods through which they can be used by {@link Command}s. Subsystems are used by the - * {@link CommandScheduler}'s resource management system to ensure multiple robot actions are not + * {@link CommandScheduler}'s subsystem management system to ensure multiple robot actions are not * "fighting" over the same hardware; Commands that use a subsystem should include that subsystem in - * their {@link Command#getRequirements()} method, and resources used within a subsystem should + * their {@link Command#getRequirements()} method, and subsystems used within a subsystem should * generally remain encapsulated and not be shared by other parts of the robot. * *

Subsystems must be registered with the scheduler with the {@link @@ -22,4 +18,65 @@ *

This class is provided by the NewCommands VendorDep */ public interface Subsystem extends Resource { + /** + * Gets the name of this subsystem for telemetry. + * + * @return resource name + */ + default String getName() { + return this.getClass().getSimpleName(); + } + + /** + * This method is called periodically by the {@link CommandScheduler}. Useful for updating + * subsystem-specific state that you don't want to offload to a {@link Command}. Teams should try + * to be consistent within their own codebases about which responsibilities will be handled by + * Commands, and which will be handled here. + */ + default void periodic() {} + + /** + * This method is called periodically by the {@link CommandScheduler}. Useful for updating + * subsystem-specific state that needs to be maintained for simulations, such as for updating + * {@link edu.wpi.first.wpilibj.simulation} classes and setting simulated sensor readings. + */ + default void simulationPeriodic() {} + /** + * Sets the default {@link Command} of the subsystem. The default command will be automatically + * scheduled when no other commands are scheduled that require the subsystem. Default commands + * should generally not end on their own, i.e. their {@link Command#isFinished()} method should + * always return false. Will automatically register this subsystem with the {@link + * CommandScheduler}. + * + * @param defaultCommand the default command to associate with this subsystem + */ + default void setDefaultCommand(Command defaultCommand) { + CommandScheduler.getInstance().setDefaultCommand(this, defaultCommand); + } + + /** + * Removes the default command for the subsystem. This will not cancel the default command if it + * is currently running. + */ + default void removeDefaultCommand() { + CommandScheduler.getInstance().removeDefaultCommand(this); + } + + /** + * Gets the default command for this subsystem. Returns null if no default command is currently + * associated with the subsystem. + * + * @return the default command associated with this subsystem + */ + default Command getDefaultCommand() { + return CommandScheduler.getInstance().getDefaultCommand(this); + } + + /** + * Registers this subsystem with the {@link CommandScheduler}, allowing its {@link + * Subsystem#periodic()} method to be called when the scheduler runs. + */ + default void register() { + CommandScheduler.getInstance().registerSubsystem(this); + } } diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/SubsystemBase.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/SubsystemBase.java index 469b13d492a..3a024e5f665 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/SubsystemBase.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/SubsystemBase.java @@ -14,14 +14,14 @@ * *

This class is provided by the NewCommands VendorDep */ -public abstract class SubsystemBase implements Resource, Sendable { +public abstract class SubsystemBase implements Subsystem, Sendable { /** Constructor. Telemetry/log name defaults to the classname. */ @SuppressWarnings("this-escape") public SubsystemBase() { String name = this.getClass().getSimpleName(); name = name.substring(name.lastIndexOf('.') + 1); SendableRegistry.addLW(this, name, name); - CommandScheduler.getInstance().registerResources(this); + CommandScheduler.getInstance().registerSubsystem(this); } /** @@ -32,7 +32,7 @@ public SubsystemBase() { @SuppressWarnings("this-escape") public SubsystemBase(String name) { SendableRegistry.addLW(this, name, name); - CommandScheduler.getInstance().registerResources(this); + CommandScheduler.getInstance().registerSubsystem(this); } /** diff --git a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/sysid/SysIdRoutine.java b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/sysid/SysIdRoutine.java index cb6d9305b96..5bd0cbed209 100644 --- a/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/sysid/SysIdRoutine.java +++ b/wpilibNewCommands/src/main/java/edu/wpi/first/wpilibj2/command/sysid/SysIdRoutine.java @@ -19,7 +19,6 @@ import edu.wpi.first.wpilibj.sysid.SysIdRoutineLog; import edu.wpi.first.wpilibj2.command.Command; import edu.wpi.first.wpilibj2.command.Resource; - import java.util.Map; import java.util.function.Consumer; diff --git a/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/SchedulerTest.java b/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/SchedulerTest.java index d175e52df6e..50b793eb3fa 100644 --- a/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/SchedulerTest.java +++ b/wpilibNewCommands/src/test/java/edu/wpi/first/wpilibj2/command/SchedulerTest.java @@ -120,7 +120,7 @@ void schedulerInterruptCauseLambdaInRunLoopTest() { void registerSubsystemTest() { try (CommandScheduler scheduler = new CommandScheduler()) { AtomicInteger counter = new AtomicInteger(0); - Resource system = + Subsystem system = new SubsystemBase() { @Override public void periodic() { @@ -128,7 +128,7 @@ public void periodic() { } }; - assertDoesNotThrow(() -> scheduler.registerResources(system)); + assertDoesNotThrow(() -> scheduler.registerSubsystem(system)); scheduler.run(); assertEquals(1, counter.get()); @@ -139,14 +139,14 @@ public void periodic() { void unregisterSubsystemTest() { try (CommandScheduler scheduler = new CommandScheduler()) { AtomicInteger counter = new AtomicInteger(0); - Resource system = + SubsystemBase system = new SubsystemBase() { @Override public void periodic() { counter.incrementAndGet(); } }; - scheduler.registerResources(system); + scheduler.registerSubsystem(system); assertDoesNotThrow(() -> scheduler.unregisterSubsystem(system)); scheduler.run();